ImageGen models support for --pull #3302
Conversation
| std::string maxResolution = ""; | ||
| std::string defaultResolution = ""; | ||
| std::optional<uint32_t> maxNumberImagesPerPrompt; |
There was a problem hiding this comment.
Default = optional not empty?
There was a problem hiding this comment.
By default it won't be present in graphpbtxt. And by default optional will be nullopt
| node_options: { | ||
| [type.googleapis.com / mediapipe.ImageGenCalculatorOptions]: { | ||
| models_path: "./" | ||
| target_device: "CPU" |
There was a problem hiding this comment.
Wouldn't it be better if we set this default in proto?
There was a problem hiding this comment.
I think this is specific parameter, that makes it easy for an admin to see where the models will be loaded. Default in proto is also good, as a fallback. I liked this approach and copied it from rerank and embeddings behavior. What do you think about it?
| ASSERT_FALSE(imageGenerationGraphSettings.maxNumInferenceSteps.has_value()); | ||
| ASSERT_TRUE(imageGenerationGraphSettings.pluginConfig.empty()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing test for incorrect resolution?
There was a problem hiding this comment.
The tests exists, in cli parser death tests. At this point the resolution will always be correct, otherwise cli parsing fails.
There was a problem hiding this comment.
negativeImageGenerationGraph_MaxResolutionWrongFormat
src/graph_export/graph_export.cpp
Outdated
| } | ||
| )"; | ||
|
|
||
| // TODO: Remaining params |
src/test/ovmsconfig_test.cpp
Outdated
| }; | ||
| int arg_count = 10; | ||
| EXPECT_EXIT(ovms::Config::instance().parse(arg_count, n_argv), ::testing::ExitedWithCode(OVMS_EX_USAGE), | ||
| "error parsing options: Argument ‘hello’ failed to parse"); |
There was a problem hiding this comment.
Default cxxopt behaviour - not needed test.
src/test/ovmsconfig_test.cpp
Outdated
| }; | ||
| int arg_count = 10; | ||
| EXPECT_EXIT(ovms::Config::instance().parse(arg_count, n_argv), ::testing::ExitedWithCode(OVMS_EX_USAGE), | ||
| "error parsing options: Argument ‘hello’ failed to parse"); |
There was a problem hiding this comment.
Default cxxopt behaviour - not needed test.
🛠 Summary
CVS-166467
🧪 Checklist